-
Notifications
You must be signed in to change notification settings - Fork 169
feat(tools,forks): Extend EEST to support EIP-7928 payload #1866
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feat/block-access-list
Are you sure you want to change the base?
Conversation
Unsure what this error is about:
|
I haven't modified the engine API payload yet because there are no specifications for it. I can add it if it would help clients prototype more effectively. cc: @nerolation - We need to update the Engine API specs once the EIP is SFI'd. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for implementing this.
I left a couple of comments that should help clarify how these kinds of test should be written IMO.
@@ -114,6 +114,9 @@ class Environment(EnvironmentGeneric[ZeroPaddedHexNumber]): | |||
withdrawals: List[Withdrawal] | None = Field(None) | |||
extra_data: Bytes = Field(Bytes(b"\x00"), exclude=True) | |||
|
|||
# EIP-7928: Block-level access lists | |||
bal_hash: Hash | None = Field(None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel this belongs to theResult
returned from the transition tool after executing the state transition.
Environment
is more for things that affect the EVM execution and/or are observable in the EVM context.
If we put it in Result
, we can catch it in here:
execution-spec-tests/src/ethereum_test_specs/blockchain.py
Lines 522 to 533 in 485ff27
header = FixtureHeader( | |
**( | |
transition_tool_output.result.model_dump( | |
exclude_none=True, exclude={"blob_gas_used", "transactions_trie"} | |
) | |
| env.model_dump(exclude_none=True, exclude={"blob_gas_used"}) | |
), | |
blob_gas_used=blob_gas_used, | |
transactions_trie=Transaction.list_root(txs), | |
extra_data=block.extra_data if block.extra_data is not None else b"", | |
fork=fork, | |
) |
and then even verify it during the test by using
header_verify
as in this example here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @marioevz I see this differently. The test supplies the following to the client as part of a block:
- A list of transactions
- SSZ encoded block access list (BAL)
- BAL hash
In the PR (1) and (2) is part of a test. (3) is computed by the framework from (2).
State transition
The client's state transition function takes all three to produce a block. In addition to executing the transactions, the client:
- Computes its own copy BAL and then
- Computes BAL hash
The Client and NOT test verifies hash
The client MUST reject the block if computed hash does not match the provided one. ref: spec
If hash is not supplied as an input to the client it will not be able to perform this check. Hence its inclusion in ENV
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know if I misunderstood anything in this flow.
We are trying to fill tests here to generate a test fixture with a built block to then test against clients. So we are essentially testing the EL's ability to locally build a valid block when we're filling tests. This block is then sent as a payload to a client via the test fixture. So I feel like we could actually perform the check at the Result
level here. If we build our BALs and compute our own hash to check against, what we need from the filler (let's say this is EELS) is that they compute their own BALs internally, generate the hash, and we validate that this hash is the expected hash in the result with header_verify=Header(bal_hash=correct_bal_hash)
. And we can test any invalid cases with rlp_modifier=Header(bal_hash=invalid_bal_hash)
and exception=BlockException.INVALID_BLOCK_HASH
(or a more appropriate bal hash exception).
The client MUST reject the block if computed hash does not match the provided one. ref: spec
I think this comes into play when we're executing the test payload against a client. They will now have a filled block in the test fixture, with the hash that we validated is correct, and they should indeed correctly raise on an invalid hash according to the spec.
Am I missing anything @raxhvl? Does this go along with what you were thinking @marioevz?
) | ||
|
||
# Create block with custom header that includes BAL hash | ||
block = Block(txs=[tx], block_access_lists=bal.serialize()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be more of a verification that the BAL-specific tests do, rather than something you have to pass on every test.
There's two ways of handling BALs in tests IMO:
- The test is unconcerned with BALs, so we take the value that was returned from the transition tool for granted and just put it in the result.
- The test is meant to verify BALs, so we add the
header_verify
field when constructing the block for the result from the transition tool to be cross-checked with the one that the test expects.
The BAL will be either way be verified at client level when the test is being executed either in hive or by the client consumer, and the test should fail if the BAL hash does not match what the filled test says.
Taking into account, I think this specific example should be rewritten as something like:
block = Block(txs=[tx], block_access_lists=bal.serialize()) | |
block = Block(txs=[tx], header_verify=Header(bal_hash=bal.serialize().hash())) |
Assuming bal.serialize().hash()
returns the hash that should be placed in the header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Continuing the discussion, client simply rejects a block with invalid hash. For the test, client behaves like a black box which takes all three inputs to either accept or reject the block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just getting to this comment now but this agrees with the way I'm thinking about it as well as I described above.
The client will build its own BAL and compute its own hash. We do so internally in the test and will need validate that it's correct after filling.
So what we need is a built BAL that can be hashed in the appropriate way (this can be bal.hash
, bal.serialize().hash()
, or however we want to wire it) and we need to check here that the hash is as expected. When the client runs this filled test against it, the payload from the fixture is built correctly (or if invalid will have an expect_exception
) and this will test that the client independently builds its own BAL and hashes appropriately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a first pass here @raxhvl but I want to make sure I'm on the right track. If you can link the pokebal
dependency that would be great. I also think we should adjust the naming block_access_lists
(anywhere this is found) -> block_access_list
(EIP syntax).
tests/unscheduled/eip7928_block_level_access_lists/test_validate_bal.py
Outdated
Show resolved
Hide resolved
tests/unscheduled/eip7928_block_level_access_lists/test_validate_bal.py
Outdated
Show resolved
Hide resolved
@@ -114,6 +114,9 @@ class Environment(EnvironmentGeneric[ZeroPaddedHexNumber]): | |||
withdrawals: List[Withdrawal] | None = Field(None) | |||
extra_data: Bytes = Field(Bytes(b"\x00"), exclude=True) | |||
|
|||
# EIP-7928: Block-level access lists | |||
bal_hash: Hash | None = Field(None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know if I misunderstood anything in this flow.
We are trying to fill tests here to generate a test fixture with a built block to then test against clients. So we are essentially testing the EL's ability to locally build a valid block when we're filling tests. This block is then sent as a payload to a client via the test fixture. So I feel like we could actually perform the check at the Result
level here. If we build our BALs and compute our own hash to check against, what we need from the filler (let's say this is EELS) is that they compute their own BALs internally, generate the hash, and we validate that this hash is the expected hash in the result with header_verify=Header(bal_hash=correct_bal_hash)
. And we can test any invalid cases with rlp_modifier=Header(bal_hash=invalid_bal_hash)
and exception=BlockException.INVALID_BLOCK_HASH
(or a more appropriate bal hash exception).
The client MUST reject the block if computed hash does not match the provided one. ref: spec
I think this comes into play when we're executing the test payload against a client. They will now have a filled block in the test fixture, with the hash that we validated is correct, and they should indeed correctly raise on an invalid hash according to the spec.
Am I missing anything @raxhvl? Does this go along with what you were thinking @marioevz?
🗒️ Description
A sub-PR of #205 that introduces following changes to the framework:
BlockAccessLists
bal_hash
to block headerblock_access_lists
to block bodybal_hash
andblock_access_lists
to t8n environment, and test fixtures.Implementation of EIP-7928 engine API specs
The implementation of EIP-7928 requires passing of the ssz encoded block access list to be passed as a parameter of engine API specifications. The framework's engine API specs has been updated based on the proposed update to engine API specs.
References
🔗 Related Issues or PRs
closes #1836
✅ Checklist
tox
checks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:uvx --with=tox-uv tox -e lint,typecheck,spellcheck,markdownlint
type(scope):
.